Skip to content

Improve window frames#5639

Merged
martint merged 1 commit intotrinodb:masterfrom
kasiafi:MinorOperatorCleanup
Oct 27, 2020
Merged

Improve window frames#5639
martint merged 1 commit intotrinodb:masterfrom
kasiafi:MinorOperatorCleanup

Conversation

@kasiafi
Copy link
Member

@kasiafi kasiafi commented Oct 21, 2020

No description provided.

@cla-bot cla-bot bot added the cla-signed label Oct 21, 2020
@martint martint self-requested a review October 22, 2020 00:09
@kasiafi kasiafi force-pushed the MinorOperatorCleanup branch 2 times, most recently from 9d60569 to 7891510 Compare October 25, 2020 17:33
@kasiafi kasiafi force-pushed the MinorOperatorCleanup branch from 7891510 to b9979c3 Compare October 27, 2020 11:49
@martint martint merged commit 629c1cd into trinodb:master Oct 27, 2020
@martint martint added this to the 346 milestone Oct 27, 2020
@martint martint mentioned this pull request Oct 27, 2020
10 tasks
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Oct 26, 2021
Cherry-pick of trinodb/trino#5639

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Oct 29, 2021
Cherry-pick of trinodb/trino#5639

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
v-jizhang added a commit to v-jizhang/presto that referenced this pull request Jan 2, 2023
Cherry-pick of trinodb/trino#5639

Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
feilong-liu added a commit to feilong-liu/presto that referenced this pull request Feb 16, 2023
Cherry-pick of trinodb/trino#5639

Co-authored-by: Jinlin Zhang <z_jinlin@yahoo.com>
Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
feilong-liu added a commit to prestodb/presto that referenced this pull request Feb 17, 2023
Cherry-pick of trinodb/trino#5639

Co-authored-by: Jinlin Zhang <z_jinlin@yahoo.com>
Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
wanglinsong pushed a commit to prestodb/presto that referenced this pull request Feb 21, 2023
Cherry-pick of trinodb/trino#5639

Co-authored-by: Jinlin Zhang <z_jinlin@yahoo.com>
Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
sortKeyChannelForEndComparison = Optional.of(source.getLayout().get(frame.getSortKeyCoercedForFrameEndComparison().get()));
}
if (node.getOrderingScheme().isPresent()) {
sortKeyChannel = Optional.of(sortChannels.get(0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortChannels is a list. why using the first element only is sufficient?
or is the list known to be single element in this branch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortKeyChannel and ordering are only used for window frame using RANGE PRECEDING or RANGE FOLLOWING. With such frame definition, a single sort item is required per spec, and it is guaranteed by the Analyzer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the code should use getOnlyElement(sortChannels)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't work. In general, we can have multiple sort channels at this point. But for frame type RANGE, there is only one. We capture it here and use later only for RANGE frames.

I added a clarifying comment: #24157.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it has multiple sortkeyChannel for frame type RANGE - Would we skip them ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants